feature(cli): refactor environment configuration with layered design #280
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Create runner/helpers/envs/ directory with modular configuration - base_env.sh: Base configuration (logging, cluster info, pythonpath) - common_network.sh: Network and communication settings (NCCL, RCCL) - perf_tuning.sh: Performance tuning and optimizations - primus-env.sh: Main entry point with layered loading - detect_gpu.sh: GPU model detection - MI300X.sh, MI325X.sh, MI355X.sh: GPU-specific configurations - get_ip_interface.sh, get_nccl_ib_hca.sh: Network detection utilities - Refactor configuration loading with clear responsibilities - Implement dependency checks between config files - Add validation with validate_distributed_params() - Support debug mode (PRIMUS_DEBUG=1) - Support validation skip (PRIMUS_SKIP_VALIDATION=1) - Move RCCL configuration from perf_tuning.sh to common_network.sh - Better categorization: communication vs performance - Unified location for NCCL and RCCL settings - Rename env_common_network.sh to common_network.sh - Consistent naming with other config files - Move validation function to runner/lib/validation.sh - Eliminate duplicate code with DRY principle - Remove unnecessary wrapper function (YAGNI, KISS) - Add comprehensive unit tests for primus-env.sh - 10 test cases covering all core functionality - Tests for validation, debug mode, defaults, error detection - 100% test pass rate (10/10) - Update test runner to include new test suite - Add test_primus_env.sh to run_all_tests.sh Architecture improvements: - Clear separation of concerns (base, network, perf, gpu-specific) - Better maintainability with modular design - Robust error handling and validation - Production-ready configuration system
Xiaoming-AMD
changed the title
Temporarily disable GPU-specific configurations in MI300X, MI325X, and MI355X. Keep as documentation template for future use.
Merge common_network.sh, perf_tuning.sh, and detect_gpu.sh into base files to reduce file count and simplify configuration structure. Changes: - Merge common_network.sh and perf_tuning.sh into base_env.sh - Merge detect_gpu.sh into primus-env.sh - Delete redundant files: common_network.sh, perf_tuning.sh, detect_gpu.sh - Simplify primus-env.sh loading order (2 layers instead of 4) Result: - File count: 10 → 7 files (-30%) - Cleaner directory structure - Faster loading (fewer source calls) - Better integration (GPU detection uses logging functions) - Improved error handling (GPU detection doesn't exit on failure)
wenxie-amd
approved these changes
Nov 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Create runner/helpers/envs/ directory with modular configuration
Refactor configuration loading with clear responsibilities
Add comprehensive unit tests for primus-env.sh
Update test runner to include new test suite
Architecture improvements: